Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make checksum propfind future proof #22199

Merged
merged 1 commit into from
Feb 9, 2016
Merged

Make checksum propfind future proof #22199

merged 1 commit into from
Feb 9, 2016

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Feb 8, 2016

The checksum PR from #21997 returns a propfind like:

<oc:checksum>TYPE:CHECKSUM</oc:checksum>

Which if we will ever support multiple cheksums results in:

<oc:checksum>TYPE1:CHECKSUM1,TYPE2:CHECKSUM2</oc:checksum>

Which is not very webdavvy.

This PR moves that to:

<oc:checksums>
  <oc:checksum>TYPE1:CHECKSUM1</oc:checksum>
  <oc:checksum>TYPE2:CHECKSUM2</oc:checksum>
</oc:checksum>

Which is a bit more future proof.

Testing is basically the same as: #21997 (comment) with the exception that your propfind file should contain:

<?xml version="1.0"?>
<a:propfind xmlns:a="DAV:">
<a:prop xmlns:oc="http://owncloud.org/ns">
    <oc:checksums/>
</a:prop>
</a:propfind>

CC: @PVince81 @dragotin @MorrisJobke @nickvergessen

@rullzer
For possible future usage allow multiple checksums
5486a2b 
Instead of returning

<oc:checksum>TYPE:CHECKSUM</oc:checksum>

Return

<oc:checksums>
  <oc:checksum>TYPE:CHECKSUM</oc:checksum>
</oc:checksums>

This will allow us to expand in the future to multiple checksums.
Without doing just string concatenation.

And even for a single checksum it is now correct.
@rullzer rullzer added this to the 9.0-current milestone Feb 8, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @PVince81, @DeepDiver1975 and @LukasReschke to be potential reviewers

@MorrisJobke
Copy link
Contributor 

(multiple_checksums) $ curl -u mjob:mjob -X PROPFIND http://localhost/master/remote.php/webdav/foobar --upload-file prop
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/master/remote.php/webdav/foobar</d:href>
  <d:propstat>
   <d:prop>
    <oc:checksum/>
   </d:prop>
   <d:status>HTTP/1.1 404 Not Found</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>
(multiple_checksums) $ git checkout master                                                                                       
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
(master) $ curl -u mjob:mjob -X PROPFIND http://localhost/master/remote.php/webdav/foobar --upload-file prop
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/master/remote.php/webdav/foobar</d:href>
  <d:propstat>
   <d:prop>
    <oc:checksum>MD5:12i34769781236496913,sha1:asdoöiadshfölha</oc:checksum>
   </d:prop>
   <d:status>HTTP/1.1 200 OK</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>

@MorrisJobke
Copy link
Contributor

@rullzer Seem to be broken. :(

@MorrisJobke
Copy link
Contributor

Tested with the updated file prop and it works now 👍

@karlitschek
Copy link
Contributor

I'm O.K.with that. But please discuss architecture question like that with @DeepDiver1975 and me in the future.
👍

DeepDiver1975 added a commit that referenced this pull request Feb 9, 2016
@DeepDiver1975
Merge pull request #22199 from owncloud/multiple_checksums
98497aa 
Make checksum propfind future proof
@DeepDiver1975 DeepDiver1975 merged commit 98497aa into master Feb 9, 2016
@DeepDiver1975 DeepDiver1975 deleted the multiple_checksums branch February 9, 2016 08:10
@MorrisJobke MorrisJobke mentioned this pull request Feb 9, 2016
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
4 - To release app:dav
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rullzer @mention-bot @MorrisJobke @karlitschek @DeepDiver1975